fix(netty): preserve all redirect body types - #2316
Merged
Merged
Conversation
6 tasks
Contributor
Author
|
We use ahc in https://github.com/playframework/play-ws and I am in the process of upgrading to v3 - and found some thing worth adressing. |
hyperxpro
requested changes
Aug 26, 2026
Redirect30xInterceptor copied only six request-body representations when a redirect retained the body. Composite byte arrays, ByteBufs, InputStreams, and Files therefore became empty requests on 307, 308, and strict 302 redirects. Copy the representation selected for the original request, following NettyRequestFactory's precedence. Resettable streams can then replay, while non-resettable streams fail promptly through the existing replay guard. Cover all four omissions byte-for-byte, pin coexistence precedence, retain the caller-owned ByteBuf, and keep form and multipart replay behavior. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Build keep-body redirects from the original request so every supported body representation and per-request option follows the redirect without duplicating NettyRequestFactory's selection logic. Clear target-specific routing and credential state when the origin changes, and copy headers before removing redirect-only values. Cover read timeout, range offset, and Cookie object handling. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Close the test InputStream and assert the precise replay failure instead of accepting any IOException. This pins the intended fail-fast behavior for a consumed non-resettable stream. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Exercise a real file-backed InputStream across a keep-body redirect. The first send closes the stream, so the replay must fail promptly with the documented IOException instead of sending an empty body. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Keep an explicit Content-Length when replaying a raw InputStream. Unlike other body representations, the stream has no intrinsic size from which the redirect request can recompute the header. Verify both request legs receive the same length and body bytes. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Reject keep-body redirects containing InputStreamPart before constructing the second request. Multipart stream parts are closed after the first send and have no replay guard, so attempting to reuse them can hang or send incomplete content. Cover the failure with a real file-backed multipart stream. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
Validate the file body actually selected by the original request before dispatching a keep-body redirect. Report a checked IOException before the redirect path can remove a pooled channel and fail with an unchecked constructor exception. Preserve body-selection precedence when a sticky File coexists with a higher-priority representation. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
mkurz
force-pushed
the
fix/redirect-body-replay
branch
from
August 27, 2026 22:14
8176027 to
6268d4d
Compare
4 tasks
hyperxpro
requested changes
Aug 31, 2026
The previous cross-origin Cookie-object test used GET with a 302, so the fresh builder dropped cookies without exercising resetCookies. Use a 307 POST and observe both redirect legs so the test fails if toBuilder copies the Cookie object to the new origin. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
An InputStreamBodyGenerator without a declared length builds the same unknown-length Netty body as a raw InputStream. Removing an explicit Content-Length therefore made only the redirected leg use chunked encoding. Detect the selected body representation using request-factory precedence and retain the caller-supplied length for either unknown-length stream form. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
The existing settings regression used a 307 and therefore inherited readTimeout and rangeOffset through Request.toBuilder. Add a 303 case so the fresh-builder path is also required to copy both per-request values. OpenAI Codex on behalf of Matthias Kurz. Co-Authored-By: OpenAI Codex <codex@openai.com>
hyperxpro
requested changes
Aug 31, 2026
Co-authored-by: Aayush Atharva <24762260+hyperxpro@users.noreply.github.com>
hyperxpro
approved these changes
Aug 31, 2026
Member
|
Thanks a lot! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Content-Lengthwhen replaying a rawInputStreamor an unknown-lengthInputStreamBodyGenerator.ByteBufreferences, and the new failure modes with focused tests.Problem
Redirect30xInterceptorrebuilds a request when it follows a strict 302, 307, or 308 redirect. Its keep-body copy chain handled form parameters, strings, byte arrays,ByteBuffer, body generators, and multipart bodies, but omitted four real request send paths:List<byte[]>/ composite byte arraysByteBufInputStreamFileThe redirected request therefore kept its method and Content-Type but sent zero bytes. The
Filecase is especially risky for uploads because the target can accept an apparently valid empty PUT or POST. Reconstructing the request field by field also omitted unrelated per-request state such as the read timeout and range offset, and maintaining a second body-selection chain alongsideNettyRequestFactorymade future drift likely.This is a pre-existing omission. AHC issue #1643 previously fixed the same class of bug for multipart bodies. The copy chain was carried through pull request #1843 without a policy discussion. Focused searches found no existing issue or pull request covering these four representations.
Change
Build a keep-body redirect with
request.toBuilder()and then replace only redirect-specific state. This preserves all current and future body representations and per-request options without duplicatingNettyRequestFactory.body. Headers are copied before redirect-only values are removed, so the original request is not mutated.On a cross-origin redirect, the copied request drops the previous resolved address, virtual host, realm, authorization headers, and Cookie objects before the cookie store adds cookies that legitimately match the new URI. The body is not covered by that boundary. It follows the existing keep-body policy, which means a
FileorInputStreambody that a cross-origin redirect leg previously received as empty is now sent in full, and a target that keeps redirecting can receive it once per hop up tomaxRedirects. That is the same exposure byte arrays, strings, form parameters, and multipart bodies already have today.Composite byte arrays, caller-owned
ByteBufs, and files are repeatable. A resettableInputStream, such asByteArrayInputStream, also replays. A caller-suppliedContent-Lengthis retained for a rawInputStreamor anInputStreamBodyGeneratorwithout a declared length, because neither has an intrinsic size from which to recompute it. A consumed stream that cannot be reset reaches the existing fail-fast guard added in #2312 and completes the future withIOException; that is preferable to silently succeeding with an empty body.An
InputStreamPartis closed by the first multipart send and has no equivalent replay guard, so a keep-body redirect now fails promptly instead of risking a hang or incomplete multipart request. A selectedFileorFileBodyGeneratoris also checked before dispatching the redirect; if it disappeared after the first send, the future fails withIOExceptionbefore a target pooled channel can be removed and an unchecked constructor exception can escape. The validation followsNettyRequestFactoryprecedence so a stickyFilefield is ignored when a higher-priority body representation was actually sent.The change does not alter which methods or status codes keep a body, nor does it introduce a new cross-origin policy. It makes the existing strict-302, 307, and 308 behavior complete for every supported request-body representation.
Compatibility
There is no public API change. Requests that previously sent an empty body on a keep-body redirect now resend their configured body.
Behavior changes:
InputStreamon a keep-body redirect previously completed successfully after sending an empty redirected request. It now completes the request future exceptionally withIOException. This includesFileInputStream, which is closed after the first send and cannot be reset for replay.InputStreamPartnow fails promptly withIOExceptionwhen a keep-body redirect requires replay. Reusing its already-consumed and closed stream could previously hang or send incomplete multipart content.IOExceptionbefore redirect dispatch rather than allowing an uncheckedIllegalArgumentExceptionto escape while constructing the next request.FileorInputStreambody is now sent on a keep-body redirect to a different origin, where the redirected request previously carried no body. Credentials are still stripped at that boundary, but the payload is not.Callers that accidentally relied on an empty or incomplete redirected request will observe an exception, but the failure is explicit instead of silently losing configured content. There is no public API change.
AI disclosure
OpenAI Codex on behalf of Matthias Kurz. The commit includes
Co-Authored-By: OpenAI Codex <codex@openai.com>perAGENTS.md.Test plan
upstream/main, the focused suite reproduced five failures: four body types arrived as zero bytes and a non-resettable stream incorrectly completed successfully.InputStreamBodyGeneratorsentContent-Length: 13on the first leg and noContent-Lengthon the redirected leg../mvnw -pl client -Dtest=RedirectBodyTest,RedirectCredentialSecurityTest teston JDK 11: 40 tests passed, including Netty leak detection../mvnw clean verifyon JDK 11: 1,496 tests passed and Revapi completed without failures (BUILD SUCCESS).Generated with OpenAI Codex.